Skip to content

(🎁) ci: include more of the primer comment#15032

Closed
KotlinIsland wants to merge 1 commit into
python:masterfrom
KotlinIsland:ci/unlimited
Closed

(🎁) ci: include more of the primer comment#15032
KotlinIsland wants to merge 1 commit into
python:masterfrom
KotlinIsland:ci/unlimited

Conversation

@KotlinIsland

@KotlinIsland KotlinIsland commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

I don't think you need to truncate the primer comment anymore.

proof: here we have applied the same change, removing the truncation of the comment, which is having no issue commenting the full-form of the diff.

@github-actions

This comment has been minimized.

@ikonst ikonst left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All -s, nice!

@ikonst

ikonst commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

@ikonst

ikonst commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Actually, are you sure that KotlinIsland/basedmypy#357 (comment) is pushing the limit?

In particular, in #14059 there are two issues being discussed:

  • comments that are long enough to be rejected by GitHub API
  • a single project "monopolizing" the report

@ikonst

ikonst commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Trial and error shows that this is the longest comment GitHub would accept:

python3 -c 'print("x" * (2**18 - 2))'  | pbcopy

Meanwhile, the UI says the maximum length is 65,535 characters, which is probably isn't far from the truth when taking Unicode into consideration.

@KotlinIsland KotlinIsland marked this pull request as draft April 12, 2023 09:16
@KotlinIsland KotlinIsland changed the title (🎁) ci: don't truncate primer comment (🎁) ci: include more of the primer comment Apr 12, 2023
@KotlinIsland

Copy link
Copy Markdown
Contributor Author

Actually, are you sure that KotlinIsland/basedmypy#357 (comment) is pushing the limit?

Ergh, I knew it was too good to be true :(. So basically the problem with the current solution is that I don't see all the diff, I can see three solutions:

  1. Include more of the diff to fill out the comment limit, not a full form solution, as a big diff could over fill it.
  2. comment multiple times
  3. when the comment is truncated, include a link to the action output which will contain the full form diff.

I've updated this PR to meet option 1, happy to accept other ideas.

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@ikonst

ikonst commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

A "core dump" in a comment is not what comments were made for, so GitHub's UI is not particularly accommodating for that use case, and it can also interfere with our normal communications around PRs in comments.

Maybe opt for shorter output that only gives a "taste" for what changed, and a link to the artifact download URL, e.g. matchArtifact.archive_download_url?

@KotlinIsland

KotlinIsland commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

I think that linking to a downloadable zip is not an idea UX (although would be an improvement).

What if we always put the changes in a details? I'll put something together...

@ikonst

ikonst commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Also check that "Details" don't slow down scrolling etc. (And then maybe Details per project?)

@hauntsaninja

Copy link
Copy Markdown
Collaborator

I like the idea of including a link to artifact download if we truncated things.

@hauntsaninja

Copy link
Copy Markdown
Collaborator

Closing since this has gotten stale, feel free to reopen if you want to try link to artifact download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants